Skip to content

feat: reverse index scans and statement-scoped write visibility - #384

Merged
KKould merged 5 commits into
mainfrom
feat/reverse-index-scan
Oct 1, 2026
Merged

KKould merged 5 commits into
mainfrom
feat/reverse-index-scan

Conversation

@KKould

@KKould KKould commented Oct 1, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue link:

  1. Prepared execution cloned the whole plan per run, and executors copied operator fields.
  2. ORDER BY ... DESC / MAX always needed a full index scan plus sort/TopK; TPC-C Order-Status got slower as orders grew.
  3. Correctness bug: a scan could re-read rows written by the same statement. On LMDB, UPDATE t SET id = id + 1000 WHERE id > 3 updated rows multiple times (or until integer overflow), and INSERT INTO t SELECT ... FROM t duplicated rows. RocksDB had the same bug inside explicit transactions.
  4. Memory storage cloned the entire key range at scan start.

What is changed and how it works?

  • Borrowed plans: executors borrow &LogicalPlan operators instead of copying them; prepared statements no longer clone the plan. Parameters are bound lazily in IndexScan.
  • Reverse index scan: range_rev on all storages; the optimizer reverses an index scan when the whole required order is reversed (NOT NULL columns only, never below a Limit). EXPLAIN shows Reverse.
  • Statement stamps: every tuple/index value gets an 8-byte suffix with the id of the statement that wrote it; scans skip entries with the current statement's stamp. Stamps come from the LMDB txn id, the RocksDB sequence number, or a memory counter, and are only allocated for statements that both scan and write (bulk INSERT ... VALUES uses 0).
  • Storage iterators: LMDB/RocksDB iterators are driven by explicit step enums; LMDB remove_range reuses the scan; memory iterates the BTreeMap lazily.
  • Tests/tooling: SLT runs on LMDB (no longer builds RocksDB); the TPC-C runner pins to the fastest P-core; README trimmed.

Code changes

  • Has Rust code change
  • Has CI related scripts change

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Note for reviewer

  • On-disk format changed (stamp suffix): data written by 0.4.0 cannot be read. Version bumped to 0.4.1.
  • Performance (TPC-C LMDB, 60 s, pinned to cpu8, 3 rounds): TpmC about -2.5% vs main; Order-Status p90 about -15% (51→43 µs). With the stamp check disabled the result is the same, so the cost comes from elsewhere in the change, most likely the extra 8 bytes per value (not profiled). RocksDB Delivery p90 dropped about 34% in a separate short run.
  • Tests: all unit tests pass; SLT 197/197 on LMDB; regression cases in update.slt / insert.slt and a shared explicit-transaction test on memory, LMDB, and RocksDB (pessimistic + optimistic) fail without the fix.
  • unsafe: PlanKeeper, the memory iterator value pointer, mdb_txn_id, and the RocksDB snapshot sequence number, each with a SAFETY comment. Not run under Miri.
  • Known gaps: equality point lookups (Range::Eq hits) are not filtered by stamp; table-level PRIMARY KEY (a, b) does not mark columns NOT NULL, so reverse scans don't apply there.

Executors now hold `&'a LogicalPlan` / `&'a Operator` and copy only the fields
they must own, so preparing a plan no longer clones the whole tree per execution.

* build_read/build_write take `&'a LogicalPlan`; Input types borrow operators
* PlanKeeper keeps an owned plan alive at a stable address for the executor
* bind prepared parameters lazily in IndexScan; ParamArena keeps (id, expr)
  slots and exposes them through MetaArena::bound_param
* IndexRanges is a cursor over borrowed ranges (owned only for runtime probes)
* SeqScan/IndexScan and DDL executors drop Option+take; DDL and COPY FROM no
  longer return a result row
* populate the output schema for the whole plan once when it is finalized
Bump kite_sql to 0.4.1. The on-disk value format changed (every tuple
and index value now carries an 8-byte statement stamp suffix), so data
written by 0.4.0 cannot be read.

- storage: add `range_rev` to all backends; LMDB/RocksDB iterators are
  driven by explicit step enums; LMDB `remove_range` reuses the scan.
- optimizer/executor: satisfy `ORDER BY ... DESC` / `MAX` with a reverse
  index scan when the whole order is reversed (NOT NULL columns only,
  never below a Limit).
- fix: scans no longer revisit rows written by the same statement
  (e.g. primary-key UPDATE, INSERT ... SELECT on the same table, and
  the same inside explicit transactions). Stamps come from the LMDB
  txn id, the RocksDB sequence number, or a memory counter, and are
  only allocated for statements that both scan and write.
- memory: iterate the BTreeMap lazily instead of cloning the range.
- tests: SLT now runs on LMDB; tpcc runner pins to the fastest P-core.
@KKould KKould self-assigned this Oct 1, 2026
@KKould KKould added enhancement New feature or request bug Something isn't working labels Oct 1, 2026
@codecov

codecov Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.48666% with 54 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.26%. Comparing base (83ceb09) to head (30d5d8f).

Files with missing lines Patch % Lines
src/planner/arena.rs 80.00% 9 Missing ⚠️
src/storage/mod.rs 96.68% 7 Missing ⚠️
src/db/prepared.rs 91.04% 6 Missing ⚠️
src/storage/rocksdb.rs 95.97% 6 Missing ⚠️
src/optimizer/rule/normalization/elimination.rs 95.41% 5 Missing ⚠️
src/storage/lmdb.rs 96.22% 4 Missing ⚠️
src/execution/dql/scalar_apply.rs 86.95% 3 Missing ⚠️
src/planner/mod.rs 92.00% 2 Missing ⚠️
src/storage/memory.rs 97.26% 2 Missing ⚠️
src/storage/table_codec.rs 95.91% 2 Missing ⚠️
... and 7 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #384      +/-   ##
==========================================
+ Coverage   93.09%   93.26%   +0.16%     
==========================================
  Files         258      259       +1     
  Lines       47662    47741      +79     
==========================================
+ Hits        44370    44524     +154     
+ Misses       3292     3217      -75     
Flag Coverage Δ
rust 93.26% <96.48%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/binder/parser.rs 88.96% <100.00%> (ø)
src/db.rs 92.04% <100.00%> (+0.15%) ⬆️
src/execution/ddl/add_column.rs 96.96% <100.00%> (-0.23%) ⬇️
src/execution/ddl/change_column.rs 97.08% <100.00%> (-0.15%) ⬇️
src/execution/ddl/create_table.rs 100.00% <100.00%> (ø)
src/execution/ddl/create_view.rs 100.00% <100.00%> (ø)
src/execution/ddl/drop_column.rs 97.50% <100.00%> (-0.21%) ⬇️
src/execution/ddl/drop_index.rs 100.00% <100.00%> (ø)
src/execution/ddl/drop_table.rs 100.00% <100.00%> (ø)
src/execution/ddl/drop_view.rs 100.00% <100.00%> (ø)
... and 52 more

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@KKould
KKould merged commit d980efa into main Oct 1, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant